xds/internal/client: Add async gauge metrics for Connected and Resources (A78) by mbissa · Pull Request #8807 · grpc/grpc-go

mbissa · 2026-01-07T01:18:21Z

This PR leverages the async gauge framework and implements the xdsclient metrics to report number of xds resources and whether or not the xDS client currently has a working ADS stream to the xDS server along with the required labels as part of A78

RELEASE NOTES:

xds/internal/client: Add async gauge metrics for "grpc.xds_client.connected" and "grpc.xds_client.resources" as part of A78.

codecov · 2026-01-07T01:22:10Z

Codecov Report

❌ Patch coverage is 83.00000% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.07%. Comparing base (c6d5e5e) to head (24976f7).
⚠️ Report is 109 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/xds/xdsclient/clientimpl.go	40.00%	11 Missing and 1 partial ⚠️
internal/xds/clients/xdsclient/authority.go	83.33%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8807      +/-   ##
==========================================
- Coverage   83.26%   83.07%   -0.19%     
==========================================
  Files         418      411       -7     
  Lines       33004    33025      +21     
==========================================
- Hits        27480    27437      -43     
- Misses       4109     4193      +84     
+ Partials     1415     1395      -20

Files with missing lines	Coverage Δ
internal/xds/clients/xdsclient/ads_stream.go	`86.94% <100.00%> (+2.08%)`	⬆️
internal/xds/clients/xdsclient/xdsclient.go	`86.28% <100.00%> (+1.70%)`	⬆️
internal/xds/clients/xdsclient/authority.go	`79.55% <83.33%> (+0.30%)`	⬆️
internal/xds/xdsclient/clientimpl.go	`78.29% <40.00%> (-7.16%)`	⬇️

... and 113 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

internal/xds/clients/xdsclient/metrics/metrics.go

internal/xds/xdsclient/clientimpl.go

easwars · 2026-02-06T19:12:30Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces asynchronous gauge metrics for tracking xDS client connectivity and resource counts, which is a valuable addition for observability. The implementation looks mostly solid, but I've found a critical issue in the connectivity metric logic that causes it to report incorrect status. I've also included a couple of medium-severity suggestions to improve test coverage and code clarity. Please see the detailed comments below.

internal/xds/clients/xdsclient/xdsclient.go

internal/xds/clients/xdsclient/test/metrics_test.go

internal/xds/xdsclient/clientimpl.go

internal/xds/clients/config.go

internal/xds/xdsclient/clientimpl.go

internal/xds/clients/xdsclient/test/metrics_test.go

internal/xds/xdsclient/clientimpl.go

internal/xds/clients/xdsclient/xdsclient.go

easwars · 2026-02-07T06:59:49Z

internal/xds/clients/xdsclient/xdsclient.go

+	default:
+		// Fallback for initialization states
+		return "requested"
+	}


I don't see this being specified in the gRFC. Am I missing something?

We are handling that as part of the case for xdsresource.ServiceStatusRequested, right?

internal/xds/clients/xdsclient/xdsclient.go

mbissa · 2026-03-11T21:38:37Z

sequenceDiagram
    participant OS as "OTel/Stats System"
    participant Rec as "MetricsRecorder (Core)"
    participant MR as "MetricsReporter (Adapter)"
    participant XC as "XDSClient"
    participant AS as "ADS Stream"

    Note over XC, Rec: Phase 1: Registration (One-time)
    XC->>MR: RegisterAsyncReporter(reporter)
    MR->>Rec: RegisterAsyncReporter(hook)
    Rec-->>XC: metricsCleanup func

    Note over XC, AS: Phase 2: Synchronous Events & ADS Lifecycle
    XC->>AS: WatchResource(lds:foo)
    AS->>AS: NewStream() Success
    AS->>XC: established = true

    AS->>XC: Recv(Response: lds:foo)
    XC->>XC: Cache Update (Requested -> ACKed)
    
    AS->>AS: Recv() Error (Stream Breaks)
    AS->>XC: established = false
    alt if legitimate (gRFC A9)
        AS->>MR: (No Counter)
    else non-legitimate
        AS->>MR: ReportMetric(ServerFailure)
    end
    MR->>Rec: Record Counter

    Note over OS, XC: Phase 3: Asynchronous Scrape (Periodic)
    OS->>Rec: Scrape Metrics
    Rec->>MR: Trigger Hook
    MR->>XC: Report(AsyncMetricsRecorder)
    XC->>XC: reportConnectedState() --> 1 if established, else 0
    XC->>XC: reportResourceStats() --> counts by (Type, CacheState)
    XC-->>OS: Return Gauge Values (via Core)

    Note over XC, Rec: Phase 4: Cleanup
    XC->>Rec: metricsCleanup()

mbissa · 2026-03-12T05:57:46Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces asynchronous gauge metrics for tracking the xDS client's connection status and the number of cached xDS resources, as specified in gRFC A78. The implementation is well-structured, introducing new interfaces for asynchronous metric reporting and ensuring proper lifecycle management. The accompanying tests are thorough and cover various states and scenarios for the new metrics. I have a couple of minor suggestions to improve variable naming for better code clarity and maintainability.

internal/xds/clients/xdsclient/authority.go

internal/xds/clients/xdsclient/xdsclient.go

internal/xds/clients/xdsclient/metrics/metrics.go

internal/xds/clients/xdsclient/ads_stream.go

internal/xds/clients/xdsclient/authority.go

easwars · 2026-03-16T04:38:44Z

internal/xds/clients/xdsclient/xdsclient.go

+	default:
+		// Fallback for initialization states
+		return "requested"
+	}


We are handling that as part of the case for xdsresource.ServiceStatusRequested, right?

internal/xds/clients/xdsclient/xdsclient.go

internal/xds/clients/xdsclient/metrics/metrics.go

internal/xds/clients/xdsclient/authority.go

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

easwars · 2026-03-24T23:37:59Z

internal/xds/clients/xdsclient/metrics/metrics.go

+// XDSClientConnected reports the connectivity state of the ADS stream.
+// Per gRFC A78, Value is 1 if connected, 0 otherwise.
+// Labels: grpc.target, grpc.xds.server
+// grpc.target is added by asyncMetricsRecorderAdapter


This docstring is eventually for user consumption (although currently it is under a top-level internal package). Why does it matter to the user if grpc.target is added by asyncMetricsRecorderAdapter?

Also, please take a look at go/go-style/decisions#comment-sentences.

The above comments apply to here and to XDSClientResourceStats.

I added that because I misunderstood an earlier comment. fixed it now.

easwars · 2026-03-25T00:04:23Z

internal/xds/clients/xdsclient/ads_stream.go

+	firstRequest       bool                                // False after the first request is sent out.
+	firstStreamCreated bool                                // Set to true after the very first ADS stream is created.
+	streamEstablished  bool                                // Set to true when an ADS stream is established and a response is received, except for the very first stream which is set to true immediately.


What do you think about making these three fields be of type atomic.Bool and not have to acquire the lock to read/write them. Currently, there seems to be too much of locking and unlocking.

Great suggestion! done.

easwars · 2026-03-25T00:11:27Z

internal/xds/xdsclient/clientimpl.go

@@ -75,6 +75,20 @@ var (
 		Labels:      []string{"grpc.target", "grpc.xds.server"},
 		Default:     false,
 	})
+	xdsClientConnectedMetric = estats.RegisterInt64AsyncGauge(estats.MetricDescriptor{
+		Name:        "grpc.xds_client.connected",
+		Description: "A metric that is 1 if the xDS Client is connected to an xDS server, 0 otherwise.",


Nit: Similar comment about the description that it is not necessarily whether the xDS client is connected to the server or not, but more about whether it has a working ADS stream to the server.

easwars · 2026-03-25T00:12:08Z

internal/xds/clients/config.go

 	// documentation for a list of possible metrics events.
 	ReportMetric(metric any)
+
+	// RegisterAsyncReporter registers a reporter to produce metric values for


Nit in google3. internal packages are not the same in google3.

easwars · 2026-03-25T03:42:42Z

internal/xds/clients/xdsclient/test/helpers_test.go

+	r.mu.Lock()
+	defer r.mu.Unlock()
+	r.asyncReporters[reporter] = struct{}{}
+	return func() {


Please return a onceFunc, so that it is idempotent.

easwars · 2026-03-25T03:42:51Z

internal/xds/clients/xdsclient/test/helpers_test.go

+			return nil
+		}
+		// Continue if mismatch.
+


Nit: Nix newline

removed the comment and newline as docstring is added now.

easwars · 2026-03-25T03:44:26Z

internal/xds/clients/xdsclient/test/helpers_test.go

 	return nil
 }

+func (r *testMetricsReporter) waitForSpecificMetric(ctx context.Context, metricsDataWant any) error {


We already have a waitForMetric that simply reads the first metric data out of the channel and compares it with want. It is well documented. There is no documentation for this function. Why do we need this? Why does the above one not suffice? Under what circumstances would one use this? All this needs to be clarified in the docstring.

added doc string. We need this when the first metric data out of the channel is not the one we want and sequence is not guaranteed.

easwars · 2026-03-25T03:47:00Z

internal/xds/clients/xdsclient/test/metrics_test.go

+	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
+	defer cancel()
+
+	tmr := newTestMetricsReporter()


Nit: Please create this right before it is used.

Is there any reason for this to be created upfront here?

easwars · 2026-03-25T03:50:40Z

internal/xds/clients/xdsclient/test/metrics_test.go

+	resourceTypes := map[string]xdsclient.ResourceType{xdsresource.V3ListenerURL: listenerType}
+	si := clients.ServerIdentifier{
+		ServerURI:  mgmtServer.Address,
+		Extensions: grpctransport.ServerIdentifierExtension{ConfigName: "insecure"},
+	}
+	configs := map[string]grpctransport.Config{"insecure": {Credentials: insecure.NewBundle()}}


Since none of these vars are used anywhere outside of the literal struct initialization for xdsClientConfig, I would recommend that these be inlined. The literal struct initialization syntax in Go is very readable when correctly used. See: go/go-style/decisions#literal-formatting

easwars · 2026-03-25T04:07:50Z

internal/xds/clients/xdsclient/test/metrics_test.go

+		t.Fatalf("Failed to verify nacked count: %v", err)
+	}
+
+	// Wait for the does_not_exist state.


How does this work? The xDS client uses a default watch timeout of 15s? How is the resource-not-found triggered in this test?

easwars · 2026-03-25T04:08:39Z

internal/xds/clients/xdsclient/test/metrics_test.go

+		OnStreamOpen: func(_ context.Context, _ int64, _ string) error {
+			return nil
+		},
+		OnStreamRequest: func(_ int64, _ *v3discoverypb.DiscoveryRequest) error {
+			return nil
+		},


If these are not used, there is no need for them to be defined as empty funcs.

easwars · 2026-03-25T04:09:51Z

internal/xds/clients/xdsclient/test/metrics_test.go

+	unblock := make(chan struct{})
+
+	customGRPCNewClient := func(target string, opts ...grpc.DialOption) (*grpc.ClientConn, error) {
+		interceptor := func(ctx context.Context, desc *grpc.StreamDesc, cc *grpc.ClientConn, method string, streamer grpc.Streamer, cops ...grpc.CallOption) (grpc.ClientStream, error) {


There is a lot happening here. Please add comments to make it easier for the readers.

easwars · 2026-03-25T04:10:57Z

internal/xds/clients/xdsclient/test/metrics_test.go

+	// 5. NewStream OK - metric value 0
+	lis.Restart()
+
+	time.Sleep(5 * time.Second)


We can't sleep in our tests like this. It unnecessarily increases the runtime of tests. Please figure out a way to wait for the event that you need to wait for, and then proceed.

easwars · 2026-03-25T04:11:37Z

internal/xds/clients/xdsclient/test/metrics_test.go

+	defer sCancel()
+	got, err := tmr.metricsCh.Receive(sCtx)
+	if err != nil {
+		t.Fatalf("Step 5 failed: Timeout waiting for XDSClientConnected metric: %v", err)


Someone reading logs from a test failure should be able to reasonably understand why the test is failing. Things like "Step 5" does not help in that respect.

a78-xds-client-metrics-connected-resources

ec83a4e

mbissa added this to the 1.79 Release milestone Jan 7, 2026

mbissa requested a review from easwars January 7, 2026 01:18

mbissa self-assigned this Jan 7, 2026

mbissa added Type: Feature New features or improvements in behavior Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Jan 7, 2026

mbissa assigned easwars Jan 7, 2026

Pranjali-2501 modified the milestones: 1.79 Release, 1.80 Release Jan 21, 2026

easwars reviewed Feb 5, 2026

View reviewed changes

gemini-code-assist bot reviewed Feb 6, 2026

View reviewed changes

internal/xds/clients/xdsclient/xdsclient.go Outdated Show resolved Hide resolved

internal/xds/clients/xdsclient/test/metrics_test.go Show resolved Hide resolved

internal/xds/xdsclient/clientimpl.go Outdated Show resolved Hide resolved

easwars reviewed Feb 7, 2026

View reviewed changes

easwars removed their assignment Feb 7, 2026

mbissa modified the milestones: 1.80 Release, 1.81 Release Mar 6, 2026

mbissa added 2 commits March 11, 2026 20:59

review comments

b73b5d2

fixing vet

30b9c73

mbissa force-pushed the a78-xds-gauge-metrics-with-tests branch from 916bd0e to 30b9c73 Compare March 11, 2026 21:25

mbissa assigned easwars and unassigned mbissa Mar 11, 2026

gemini-code-assist bot reviewed Mar 12, 2026

View reviewed changes

internal/xds/clients/xdsclient/authority.go Outdated Show resolved Hide resolved

internal/xds/clients/xdsclient/xdsclient.go Outdated Show resolved Hide resolved

easwars reviewed Mar 16, 2026

View reviewed changes

easwars assigned mbissa and unassigned easwars Mar 16, 2026

mbissa and others added 2 commits March 20, 2026 14:29

Apply suggestion from @gemini-code-assist[bot]

67a7993

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

review comments to fix metric determination logic

795c5d1

mbissa force-pushed the a78-xds-gauge-metrics-with-tests branch from 7582c77 to 720d321 Compare March 20, 2026 11:58

review comments

f397493

mbissa force-pushed the a78-xds-gauge-metrics-with-tests branch from 720d321 to f397493 Compare March 20, 2026 12:01

making the test more verbose and leak proof

7be03bf

mbissa assigned easwars and unassigned mbissa Mar 24, 2026

easwars reviewed Mar 25, 2026

View reviewed changes

easwars assigned mbissa and unassigned easwars Mar 25, 2026

review comments

24976f7

Conversation

mbissa commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

easwars commented Feb 6, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mbissa commented Mar 11, 2026

Uh oh!

mbissa commented Mar 12, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbissa commented Jan 7, 2026 •

edited

Loading

codecov bot commented Jan 7, 2026 •

edited

Loading